feat: create minimal global config using app.manifest and app.conf by hetangmodi-crest · Pull Request #1625 · splunk/addonfactory-ucc-generator

hetangmodi-crest · 2025-03-07T14:53:12Z

Issue number:
ADDON-78488

PR Type

What kind of change does this PR introduce?

Summary

Generate minimal globalConfig.json when there is no globalConfig in source code.

Changes

If any TA doesn't have globalConfig.json in its source directory, then after building add-on we will be generating minimal globalConfig.
We are generating this minimal globalConfig using data present in app.manifest and app.conf.

Achieved 100% unit test case coverage for this PR.

User experience

After this, every TA will have globalConfig in their source code.

Checklist

If an item doesn't apply to your changes, leave it unchecked.

I have performed a self-review of this change according to the development guidelines
Tests have been added/modified to cover the changes (testing doc)
Changes are documented
PR title and description follows the contributing principles

artemrys · 2025-03-12T07:34:08Z

splunk_add_on_ucc_framework/generators/globalConfig_generator/globalConfig_generator.py

+from ..file_generator import FileGenerator
+
+
+class GlobalConfigGenerator(FileGenerator):


Can MinimalGlobalConfig inherit FileGenerator directly without this middleware?

I did think about it, but didn't proceed with it as I thought we might move the ucc-gen init command to generate globalConfig with this hierarchy, just to keep everything in same umbrella.

I think we can make it much simpler but inheriting directly, this way we can avoid having tests/unit/generators/globalConfig_generator/test_globalConfig_generator.py at all.

We can always refactor it later if we need to bring in another feature.

tests/unit/generators/globalConfig_generator/test_create_minimal_globalConfig.py

artemrys · 2025-03-12T07:37:12Z

tests/unit/generators/globalConfig_generator/test_create_minimal_globalConfig.py

+    return "test_addon"
+
+
+@patch("addonfactory_splunk_conf_parser_lib.TABConfigParser")


Why are we patching TABConfigParser?

Since _set_attributes utilizes TABConfigParser to read app.conf and we do not have an actual app.conf, I have mocked it this.

This test case tests the internal implementation.

I'd like to see the complete unit test for this functionality. If we need app.manifest and app.conf files for this to work, I'd like to see the following test:

app_conf = AppConf("...") app_manifest = AppManifest("...") minimal_global_config = MinimalGlobalConfig( ..., app_conf, app_manifest, )

If we can't write a test like this right now - we need to refactor our code urgently.

Same with the test_generate_minimal_gc test case in this file. I would like to see the test verifying the result after running the code rather than verifying how many time something was called.

tests/unit/generators/globalConfig_generator/test_globalConfig_generator.py

artemrys · 2025-03-12T07:38:24Z

tests/unit/generators/globalConfig_generator/test_globalConfig_generator_init.py

@@ -0,0 +1,12 @@
+def test___init__html():


What is the purpose of this test?

To ensure that only expected classes and modules exist in globalConfig_generator

Those modules and classes are internal, I do not expect them to be used by some other program from the outside world so I do not see a lot of value in keeping those tests.

artemrys · 2025-03-12T07:39:42Z

splunk_add_on_ucc_framework/generators/globalConfig_generator/create_minimal_globalconfig.py

+import addonfactory_splunk_conf_parser_lib as conf_parser
+
+
+class MinimalGlobalConfig(GlobalConfigGenerator):


Hm, would it make sense for this logic to be part of an existing globalConfig? This breaks the whole integration with ...Generator classes but it would be consistent with the existing class.

The idea was, since we are generating globalConfig, I kept it under ...Generator class. Moreover, if we move it to GlobalConfig class of global_config.py, we would be updating the behaviour of that class - currently, it is to parse a globalConfig and convert it to a dictionary and provide properties to access various attributes. Moving this implementation there, this class would be responsible to create a physical globalConfig file (if not present) and then continue with the current behaviour. Additionally, we would require to give the source_dir path (which induces additional changes in the tests).

I see 2 benefits of having a class MinimalGlobalConfig inherit FileGenerator.

First - we will get our documentation page updated automatically. Second - we could eventually compare generated files by us and what's in the package.

First benefit is nice to have but totally optional. Second one is absolutely great but is not applicable to the globalConfig itself.

I would imaging GlobalConfig to have a classmethod from_app_conf_and_app_manifest (the name is not the best, I agree), accept an instance of app.conf and app.manifest files as classes and produce a GlobalConfig class as a result which can be written somewhere later.

Migrated the implementation from Filegenerator to GlobalConfig class.

…lass

artemrys · 2025-03-21T14:14:12Z

splunk_add_on_ucc_framework/global_config.py

        self._original_path = global_config_path
        self.user_defined_handlers = UserDefinedRestHandlers()

+    def generate_minimal_globalconfig(


I was thinking more about something like in this PR - #1655. We introduce the second way to initialize the GlobalConfig class which takes an instance of AppConf and an instance of AppManifest classes and should return GlobalConfig as a result. We do not need to dump it straight in this method, we have dump method in the GlobalConfig class which is designed to do it.

We might need another refactor PR to handle the _original_path parameter in the GlobalConfig class.

Let's introduce this new from_app_conf_and_app_manifest method in a separate PR, you might need to make _original_path to be Optional[str] which may cause other parts of the class to be refactored.

github-actions · 2025-03-25T10:12:21Z

Code Coverage 🎉

Type	PR	Develop	Change	Status
Line Coverage	87.04%	86.90%	0.14%	🟢 Increased
Branch Coverage	80.41%	80.12%	0.29%	🟢 Increased

artemrys · 2025-03-26T06:44:09Z

splunk_add_on_ucc_framework/global_config.py

-            if self._is_global_config_yaml
-            else json.loads(config_raw)
-        )
+    def __init__(self, global_config_path: str, **kwargs: Any) -> None:


@hetangmodi-crest this conflicts with https://github.com/splunk/addonfactory-ucc-generator/pull/1655/files, please review that PR first and let's merge it.

I'd like to keep the __init__ method how it is in that PR.

artemrys · 2025-03-26T06:44:35Z

splunk_add_on_ucc_framework/global_config.py

+        source_dir: str,
+        app_manifest: app_manifest_lib.AppManifest,
+        app_conf_content: Dict[str, Any],
+    ) -> str:


It should be a classmethod and it should return "GlobalConfig".

And please make it a separate PR after #1655 is merged.

hetangmodi-crest · 2025-03-26T10:30:57Z

Closing the PR as I'll raise a new PR once PR #1655 is merged.

hetangmodi-crest · 2025-03-31T07:20:03Z

Raised the PR #1669 to cover the changes.

hetangmodi-crest added 2 commits March 7, 2025 20:17

feat: generate minimal globalConfig

61f3a20

tests: add test cases

db9ddbb

hetangmodi-crest added the enhancement New feature or request label Mar 7, 2025

hetangmodi-crest self-assigned this Mar 7, 2025

pull-request-size bot added the size/XL label Mar 7, 2025

srv-rr-github-token and others added 4 commits March 7, 2025 14:54

docs: updated docs regarding generated conf, xml and html files

b3f9548

chore: updated class names

c1225ce

docs: update doc_generator logic

9ce8468

chore: updated encoding logic

b0020db

hetangmodi-crest marked this pull request as ready for review March 10, 2025 13:10

hetangmodi-crest requested review from a team as code owners March 10, 2025 13:10

hetangmodi-crest requested review from artemrys, kkedziak-splunk, mdobecki-splunk and sgoral-splunk March 10, 2025 13:10

artemrys changed the title ~~feat : create minimal global config using app.manifest and app.conf~~ feat: create minimal global config using app.manifest and app.conf Mar 12, 2025

artemrys requested changes Mar 12, 2025

View reviewed changes

hetangmodi-crest and others added 4 commits March 13, 2025 12:50

tests: add unit test cases and updated coverage percentage

fcb7825

chore: updating coverage for Python3.10

70a0c32

Merge branch 'develop' into feat/create-minimal-globalConfig

46ea73c

Merge branch 'develop' into feat/create-minimal-globalConfig

0f4975e

hetangmodi-crest requested a review from artemrys March 17, 2025 06:05

hetangmodi-crest and others added 6 commits March 19, 2025 18:13

refactor: shifted implementation from FileGenerator to GlobalConfig c…

73c687b

…lass

docs: updated docs regarding generated conf, xml and html files

e959ada

test(unit): add test to increase coverage

8ac87e8

chore: merge branch 'develop' into feat/create-minimal-globalConfig

c7d552a

chore: remove unwanted code

9506c10

chore: update min coverage

eabca2b

artemrys mentioned this pull request Mar 21, 2025

refactor: introduce GlobalConfig.from_file method to init GlobalConfig #1655

Merged

13 tasks

artemrys reviewed Mar 21, 2025

View reviewed changes

hetangmodi-crest added 2 commits March 24, 2025 19:04

refactor: updated the generation of minimal globalConfig

a0485fd

chore: leverage dump method of GlobalConfig

57d7a85

artemrys requested changes Mar 26, 2025

View reviewed changes

hetangmodi-crest closed this Mar 26, 2025

github-actions bot locked and limited conversation to collaborators Mar 26, 2025

hetangmodi-crest deleted the feat/create-minimal-globalConfig branch March 31, 2025 07:20

		from ..file_generator import FileGenerator


		class GlobalConfigGenerator(FileGenerator):

		return "test_addon"


		@patch("addonfactory_splunk_conf_parser_lib.TABConfigParser")

		import addonfactory_splunk_conf_parser_lib as conf_parser


		class MinimalGlobalConfig(GlobalConfigGenerator):

Conversation

hetangmodi-crest commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Type

Summary

Changes

User experience

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 25, 2025

Code Coverage 🎉

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hetangmodi-crest commented Mar 26, 2025

Uh oh!

hetangmodi-crest commented Mar 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hetangmodi-crest commented Mar 7, 2025 •

edited

Loading